Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Analyze completion site re: trailing parens #680

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Analyze completion site re: trailing parens #680

wants to merge 17 commits into from
+457 −71

Conversation

jennybc
Copy link
Member

@jennybc jennybc commented Jan 29, 2025 •
edited
Loading

Addresses posit-dev/positron#1818
Addresses posit-dev/positron#2338
Closes #369 (I just came across this earlier effort that stalled out in draft from)

I think of this as a "big and dumb" solution to this problem. But it works! The discussion I want to have is basically hinted at in #1818. I suspect it's time to create some formal CompletionOption struct that gets formed (probably) in provide_completions() that contains some analysis of the completion site. This will then be passed down to all the lower level functions that marshal completions.

Summary on what we decided together:

  • We will proceed with a big small and dumb solution along these lines, to enjoy immediate improvements in our completions for ? and inside debug() and friends.
  • Open new issue for the more profound changes that are needed in the long-term to enable completions that are responsive to the completion site and how it's situated in the AST. The "don't add parentheses and parameter hints" feature from this PR is one (but not the only) capability that the new approach will facilitate. Done in Refactor some infrastructure for completions #681.
  • Add tests to this PR.
  • Remove all/most of logging before merging. (The refactoring alluded to above will allow us to build in logging in a more streamlined way. It will be nice to make it possible to dump all completions with their provenance.)

@jennybc jennybc mentioned this pull request Jan 30, 2025
Open
jennybc
jennybc commented Jan 30, 2025
View reviewed changes
.vscode/launch.json
Comment on lines +17 to +18
},
"sourceLanguages": ["rust"]
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a neutral-to-positive move for us.

crates/ark/src/lsp/completions/completion_utils.rs Outdated Show resolved Hide resolved
crates/ark/src/lsp/completions/completion_utils.rs Outdated Show resolved Hide resolved
crates/ark/src/lsp/completions/provide.rs Outdated Show resolved Hide resolved
@jennybc jennybc marked this pull request as ready for review January 30, 2025 23:52
@jennybc jennybc requested a review from DavisVaughan January 30, 2025 23:52
DavisVaughan and others added 6 commits February 6, 2025 12:41
@DavisVaughan @jennybc
Extract out node_find_containing_call()
937b351
@DavisVaughan @jennybc
Rework on top of parameter_hints() idea
4f0a741
@DavisVaughan @jennybc
Add ParameterHints enum
9175fce 
And fix a bug with imports environment functions not respecting `parameter_hints`
@DavisVaughan @jennybc
Move tests to parameter_hints.rs
68cb510 
It seems obvious to me that we are testing a feature that "officially" lives here now
@jennybc
Tweak examples probed in tests
30522fc
@jennybc
Add comment
9233108
@jennybc
Copy link
Member Author

jennybc commented Feb 6, 2025

@DavisVaughan I merged your proposals in here, tweaked tests a bit, and added a note re: binary help.

I remain intrigued by the connection to the CallNodePositionType enum, since it feels like ParameterHints::Disabled is morally equivalent to asserting that the completion site should be treated as CallNodePositionType::Value. So I end up wondering if we really need a completely new enum or could work with CallNodePositionType.

ark/crates/ark/src/lsp/completions/sources/utils.rs

Lines 94 to 168 in ca19105

pub(super) enum CallNodePositionType {
Name,
Value,
Ambiguous,
Outside,
Unknown,
}
pub(super) fn call_node_position_type(node: &Node, point: Point) -> CallNodePositionType {
match node.node_type() {
NodeType::Anonymous(kind) if kind == "(" => {
if point.is_before_or_equal(node.start_position()) {
// Before the `(`
return CallNodePositionType::Outside;
} else {
// Must be a name position
return CallNodePositionType::Name;
}
},
NodeType::Anonymous(kind) if kind == ")" => {
if point.is_after_or_equal(node.end_position()) {
// After the `)`
return CallNodePositionType::Outside;
} else {
// Let previous leaf determine type (i.e. did the `)`
// follow a `=` or a `,`?)
return call_prev_leaf_position_type(&node, false);
}
},
NodeType::Comma => return CallNodePositionType::Name,
NodeType::Anonymous(kind) if kind == "=" => return CallNodePositionType::Value,
// Like `fn(arg<tab>)` or `fn(x = 1, arg<tab>)` (which are ambiguous)
// or `fn(x = arg<tab>)` (which is clearly a `Value`)
NodeType::Identifier => return call_prev_leaf_position_type(&node, true),
_ => {
// Probably a complex node inside `()`. Typically a `Value`
// unless we are at the very beginning of the node.
// For things like `vctrs::vec_sort(x = 1, |2)` where you typed
// the argument value but want to go back and fill in the name.
if point == node.start_position() {
return call_prev_leaf_position_type(&node, false);
}
return CallNodePositionType::Value;
},
}
}
fn call_prev_leaf_position_type(node: &Node, allow_ambiguous: bool) -> CallNodePositionType {
let Some(previous) = node.prev_leaf() else {
// We expect a previous leaf to exist anywhere we use this, so if it
// doesn't exist then we return this marker type that tells us we should
// probably investigate our heuristics.
log::warn!(
"Expected `node` to have a previous leaf. Is `call_node_position_type()` written correctly?"
);
return CallNodePositionType::Unknown;
};
let after_open_parenthesis_or_comma = if allow_ambiguous {
// i.e. `fn(arg<tab>)` or `fn(x, arg<tab>)` where it can be
// ambiguous whether we are on a `Name` or a `Value`.
CallNodePositionType::Ambiguous
} else {
CallNodePositionType::Name
};
match previous.node_type() {
NodeType::Comma => after_open_parenthesis_or_comma,
NodeType::Anonymous(kind) if kind == "(" => after_open_parenthesis_or_comma,
NodeType::Anonymous(kind) if kind == "=" => CallNodePositionType::Value,
_ => CallNodePositionType::Value,
}
}

That being said, I'm happy to get this PR merged in and defer any further thought or refactoring on this matter to the work on #681.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DavisVaughan DavisVaughan Awaiting requested review from DavisVaughan

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jennybc @DavisVaughan